Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 5400 - flagging price stores as favorites #5533

Merged
merged 4 commits into from
Aug 16, 2024

Conversation

monsieurtanuki
Copy link
Contributor

What

  • Now we can flag price stores as favorites.
  • Possible questions:
    • Should we sort the favorite stores first, or is that good enough to make explicit that they are favorites with the icon? Would sound awkward to sort, as it's difficult to say "sort stores", and it would mean that any new store would not be the first displayed but would be displayed after the "old favorite" stores.
    • Should we add the favorite button in other location pages, e.g. the map? Would sound good.

Screenshot

Screenshot_1723475579

Fixes bug(s)

Files

New file:

  • favorite_location_helper.dart: Helper used to set/unset/sort stores as user favorites.

Impacted files:

  • dao_string_list.dart: added a key for favorite price stores
  • search_location_preloaded_item.dart: added a "favorite" IconButton for price locations

New file:
* `favorite_location_helper.dart`: Helper used to set/unset/sort stores as user favorites.

Impacted files:
* `dao_string_list.dart`: added a key for favorite price stores
* `search_location_preloaded_item.dart`: added a "favorite" `IconButton` for price locations
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 21 lines in your changes missing coverage. Please review.

Project coverage is 6.96%. Comparing base (4d9c7fc) to head (f4c34e1).
Report is 284 commits behind head on develop.

Files Patch % Lines
.../lib/pages/locations/favorite_location_helper.dart 0.00% 14 Missing ⚠️
...ages/locations/search_location_preloaded_item.dart 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #5533      +/-   ##
==========================================
- Coverage     9.54%   6.96%   -2.59%     
==========================================
  Files          325     400      +75     
  Lines        16411   21172    +4761     
==========================================
- Hits          1567    1474      -93     
- Misses       14844   19698    +4854     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@teolemon
Copy link
Member

teolemon commented Aug 14, 2024

  • @monsieurtanuki The sort order should be deterministic (ie order should not change all the time, possibly on date favorited ?)
  • let's merge with one place, and add it in more places as requested by you and others ?

@monsieurtanuki
Copy link
Contributor Author

The sort order should be deterministic (ie order should not change all the time, possibly on date favorited ?)

I have no problems with ordering favorites by date. That's what I implemented before removing that part.
My concern is about non favorite stores: should we display them after favorite stores? That would mean a new store I've just selected wouldn't be at the start of my store list, but just after all the favorites. Not very user-friendly, especially if you have many favorite stores.

let's merge with one place, and add it in more places as requested by you and others ?

Makes sense.

My current code and current assumption:

  1. latest selected stores first is user-friendly
  2. explicit favorite icon is good enough for detecting favorites, especially in combination with bullet 1

@teolemon
Copy link
Member

We could make the favorite part collapsible @monsieurtanuki

@monsieurtanuki
Copy link
Contributor Author

We could make the favorite part collapsible

Too fancy for me, as a user and a coder. I'd let someone else code it.

I would suggest to do it step by step and get feedbacks from real life.
Now we can flag stores as favorites: what happens for users with many stores?

  • is flagging some of them good enough?
  • should we implement different lists of stores instead?
  • should we rethink the UI altogether in order to make space for new features like GPS?

@teolemon I assume that you added prices in many many stores, and your opinion on the UX after testing the merged PR would be a good start for the next step.

@teolemon
Copy link
Member

teolemon commented Aug 16, 2024

Usually, you have several "big locations": Near my home, Near my parents, Holidays. But that suggests a more complex UI. I'm going to approve and we'll iterate @monsieurtanuki

@monsieurtanuki
Copy link
Contributor Author

Thank you @teolemon for your review!

Obviously many possible improvements.

@monsieurtanuki monsieurtanuki merged commit e7154a6 into openfoodfacts:develop Aug 16, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow starring stores
3 participants